Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: fix fs.openAsBlob, add fs.openAsBlobSync and fsPromises.openAsBlob #49759

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

LiviaMedeiros
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros commented Sep 22, 2023

fs.openAsBlob() was added in #45258 as part of Callback API despite returning Promise instead of calling callback.

I guess it should look more like this?

semver-minor PRs that contain new features and should be released in the next minor version. unless there are objections (fs.openAsBlob was marked as experimental).

cc @jasnell

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 22, 2023
@LiviaMedeiros LiviaMedeiros mentioned this pull request Sep 22, 2023
3 tasks
@LiviaMedeiros LiviaMedeiros marked this pull request as ready for review September 23, 2023 10:27
@LiviaMedeiros LiviaMedeiros added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 23, 2023
@LiviaMedeiros
Copy link
Contributor Author

cc @nodejs/fs

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

* ) => any} callback
* @returns {void}
*/
function openAsBlob(path, options, callback) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help me understand - why do we actually need an async API with a callback for this if it's always called synchronously?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to have it aligned on API level. Having a function that pretends (at least, for now) to be async is less weird that not having it in other APIs at all.

I would like to see this PR superseded or followed up with proper implementation and maybe switch from Blob to File, but judging from #45258 (comment) the former seems to be problematic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I miss @addaleax 😍 , I think she was spot on and the impl shouldn't use blocking I/O here and return a promise/take a callback that's quite confusing IMO.

Additionally I think in the case of Blob we can probably return immediately and defer access to the file to the point someone actually tries to do something with the blob. That can be synchronous and perform no I/O and would arguably be a better API (since it makes no assumptions about the file in-between creating the Blob and accessing it)

Copy link

@jimmywarting jimmywarting Sep 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer access to the file to the point someone actually tries to do something with the blob.

I think it's highly likely that they will do something with the blob if they have called openAsBlob
i think it should check if the file exist and throw a DOMexception NotFoundError otherwise.
it's not a case about if but when they will do any I/O operation after having called openAsBlob. Developers are most likely going to use it at some point. So why not just do it upfront right away rather then later? it's going to do all this work anyway

It should also guard against modifications afterwards.

const blob = openAsBlob(path)
fs.appendFileSync(path, data)
blob.text() // ups, should throw DOMException, detected mtime or size changes. 

I believe browser do also support renaming the file and moving it to another folder and still being readable afterwards. think i remember testing this a long time ago but can't remember if that was the case.

const blob = openAsBlob(path)
fs.moveSync(path, dest)
blob.text() // works

so i don't think the type, size, filename and lastModified date should be changeable after having called openAsBlob so that would involve having to stat the file and look up some properties for it.

I don't think it should be possible to call openAsBlob(path) on a path that dose not exist, create the file and then do a blob.text() afterwards. that would just be weird and unexpected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda see the point of deferring the reading operations: the methods that allow reading content like Blob.prototype.arrayBuffer, Blob.prototype.text are async, so we only need stats to be determined immediately.
In case if I plan to read it via Blob.prototype.stream, I would be surprised to see that Node.js swallows whole file and keeps it in memory before even constructing the stream. After all, it's called openAsBlob and not readAsBlob.

Copy link

@jimmywarting jimmywarting Sep 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After all, it's called openAsBlob and not readAsBlob.

Yup! Blob/File are more like FileSystemFileHandle or fs.FileHandle that just points to somewhere where it can locate and read the data from. (and what start/end offset it has)

a blob don't read anything to memory (or at least it should not - unless it's constructed from memory)

lib/fs.js Outdated
}
cbError = err;
}
callback(cbError, blob);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is to mimmic the promise API and pretend to be async this needs a process.nextTick or queueMicrotask

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2023
@nodejs-github-bot

This comment was marked as outdated.

lib/fs.js Show resolved Hide resolved
lib/fs.js Show resolved Hide resolved
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with exposing the method on fs.promises but I don't think the existing method on fs should be changed and definitely don't think there needs to be a openAsBlobSync.

@jimmywarting
Copy link

don't think there needs to be a openAsBlobSync

Care to elaborate why?

@LiviaMedeiros
Copy link
Contributor Author

I'd like to see elaboration on why we should have promise-based method in Callback API as well.

@jasnell
Copy link
Member

jasnell commented Nov 19, 2023

There's really not a lot to ellaborate on. I don't think there's value in changing the existing API and there's definitely no value in having an openAsBlobSync. I simply view this as an unnecessary change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants